-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prefix websocket path with server path #1745
Conversation
Fixes novnc#1737 and it should work for all of the different scenarios where the relative assets also work. This includes, for example: - / - /vnc.html - /vnc/ - /vnc/vnc.html It won't work for: - /vnc (but the assets don't work either, so it's consistent) - or /vnc.html/ (but this results in a 404 by default anyway)
app/ui.js
Outdated
@@ -180,7 +180,7 @@ const UI = { | |||
UI.initSetting('shared', true); | |||
UI.initSetting('view_only', false); | |||
UI.initSetting('show_dot', false); | |||
UI.initSetting('path', 'websockify'); | |||
UI.initSetting('path', window.location.pathname.substr(1) + '/../websockify'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit roundabout way of getting to where we want¹. I'd like to see something where we construct the sane, final path and hand off here.
¹ And substr()
is also deprecated. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the usage of the deprecated substr()
method.
And I agree, not sure if this is the best method, but the main idea is to get the ball rolling because this issue seems to keep being brought up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree with @vsilvar
We are facing this issue since years ago
There were some suggestions on bug #1058 that could be explored further. We don't want to hard-code any document names, though. The ideal is to mimic what the browser does when fetching linked resources. |
In my solution, I need several servers behind an nginx |
A little hacky, but this worked. Thanks |
Could this get merged at some point? For now, I am using a rather dirty fix for my docker images after the novnc installation (also works for kasmvnc):
|
Fixes #1737 and it should work for all of the different scenarios where the relative assets also work. This includes, for example:
It won't work for: